(parquet-avro) Automatically detect list encodings in AvroReadSupport - #3753
(parquet-avro) Automatically detect list encodings in AvroReadSupport#3753clairemcginty wants to merge 2 commits into
Conversation
| configuration.getBoolean(AUTO_DETECT_LIST_STRUCTURE, AUTO_DETECT_LIST_STRUCTURE_DEFAULT); | ||
|
|
||
| if (autoDetectListStructure | ||
| && configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null |
There was a problem hiding this comment.
Is it a good time to make a shift on the default value of AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE? It has caused a lot of troubles.
There was a problem hiding this comment.
it would be nice to have the more modern encoding as default! though I'm a little nervous about the implications of changing the schema for existing datasets - I know we have some downstream use cases where a single Parquet reader is reading a globbed filepattern matching multiple partitions of the same dataset - not sure what would happen if some partitions used 2-level encoding and some used 3-level encoding. Same concern about Parquet datasets that are the source of truth for external tables in Snowflake/BigQuery/etc.
maybe I can create an issue for this and request a bit more investigation on possible implications of this change?
| && configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null | ||
| && configuration.get(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS) == null) { | ||
| if (writesNewListStructure(fileSchema)) { | ||
| configuration.setBoolean(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, false); |
There was a problem hiding this comment.
This writes the inferred mode back into the shared Configuration. ParquetReader reuses that configuration across files, so after a 3-level file, a later 2-level file (especially with a projection) is still converted as 3-level. Could this stay per-file/read-context instead of mutating the caller's configuration?
There was a problem hiding this comment.
Yeah - I was a bit worried about that. Mutating the caller conf is simplest because we want to have these properties set in both init() (when computing the projection) and prepareForRead() (when computing avroSchema if it's not already set in file footer/read conf). What do you think about modifying metadata instead of conf? like:
public ReadContext init(
ParquetConfiguration configuration, Map<String, String> keyValueMetaData, MessageType fileSchema) {
MessageType projection = fileSchema;
Map<String, String> metadata = new LinkedHashMap<String, String>();
boolean autoDetectListStructure =
configuration.getBoolean(AUTO_DETECT_LIST_STRUCTURE, AUTO_DETECT_LIST_STRUCTURE_DEFAULT);
if (autoDetectListStructure
&& configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null
&& configuration.get(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS) == null) {
if (writesNewListStructure(fileSchema)) {
- configuration.setBoolean(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, false);
- configuration.setBoolean(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS, false);
+ metadata.put("inferred." + AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, "false");
+ metadata.put("inferred." + AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS, "false");
}
}
+ // ...read those properties in prepareForRead() and apply to copied ConfigurationOr - we could just re-compute writesNewListStructure(fileSchema) in both init() and prepareForRead(). that might be more straightforward overall 🤷♀️
| return false; | ||
| } | ||
| Type repeated = group.getType(0); | ||
| return !repeated.isPrimitive() |
There was a problem hiding this comment.
I may be misremembering the LIST compatibility rules, so I wanted to check this edge case: could a legacy 2-level list also use the list/element names and therefore look like this to allListStructuresAreThreeLevel? If so, would auto-detect change its Avro shape unexpectedly, or is this case ruled out by the spec or writer assumptions?
There was a problem hiding this comment.
added a few more test cases here and the auto-detect correctly returns false for those cases! lmk if you had any other cases in mind...
2 participants
Rationale for this change
parquet-avro supports writing both "old" and "new" list encodings via the parquet.avro.write-old-list-structure config. "old" encodings (aka "2-level"), which wrap the list in a
repeated group arrayschema, are the default; "new" encodings (aka "3-level") are opt-in.On the reader side, if you're using
ParquetAvroReaderto read data that was written usingParquetAvroWriter, and don't specify a projection, both type sof list encoding get parsed automatically from a combination of the file schema + theparquet.avro.schemametadata key. There's no need to setparquet.avro.write-old-list-structurekey in your Configuration.However, if you're either:
AvroReadSupport.setRequestedProjection(...)), orparquet.avro.schemametadata key),3-levle list encodings will not be parsed correctly - the reader will inject an extra nested record, named
element, into the list item type.As a reader this introduces some pain, since you have to look up the underlying file metadata of the upstream Parquet file, and risk reading incorrect data. This PR attempts to automatically detect new list encodings based on the writer file schema.
lmk what you think of this change. Automatic inference is always a bit risky, but I tried to be conservative with the approach (only set the list structure property if all list fields in the schema use 3-level encoding; don't override
parquet.avro.write-old-list-structureif the user is already setting it). any ideas for a better approach here are welcome - this is becoming more of a pain point as 3-level lists become a more popular option among other writer sdks.What changes are included in this PR?
A new read configuration property
parquet.avro.read.autoDetectListStructure(defaulting to true) that will instruct AvroReadSupport to automatically set List configuration properties based on parsing the writer file schema.Are these changes tested?
Yes, unit tests + locally on real data.
Are there any user-facing changes?
Yes, since the new property defaults to
true- it would impact anyone who's reading 3-level list data without setting theparquet.avro.write-old-list-structurekey and who's relying on/working around the incorrectly formatted data (e.g.{"locations": [{"element": {"latitude": 0.0, "longitude": 180.0}}, ...]}instead of{"locations": [{"latitude": 0.0, "longitude": 180.0}, ...]}.additionally, this change also modifies the underlying Configuration object to add the properties.